Fix stale session permissions after wh_Auth_UserSetPermissions - #481
Fix stale session permissions after wh_Auth_UserSetPermissions#481yosuke-wolfssl wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an authorization correctness issue where wh_Auth_UserSetPermissions() updated the backend user record but did not refresh the calling session’s cached permissions, allowing revoked privileges to persist until logout. It also documents and tests the new “bind immediately” behavior and introduces a core-level guard preventing non-admin sessions from setting the admin flag.
Changes:
- Update
wh_Auth_UserSetPermissions()to refreshcontext->user.permissionswhen a logged-in user changes its own permissions successfully. - Add a core-enforced policy: non-admin sessions cannot set the admin flag via
wh_Auth_UserSetPermissions(), independent of backend policy. - Add unit + integration tests to verify immediate self-demotion effects and cache-sync behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfhsm/wh_auth.h | Documents session-cache refresh behavior and the new admin-flag refusal semantics for wh_Auth_UserSetPermissions(). |
| src/wh_auth.c | Implements admin-flag refusal for non-admin sessions and refreshes the live session permission cache on successful self-targeted updates. |
| test-refactor/client-server/wh_test_auth.c | Adds an end-to-end self-demotion test and a core-only mock-backend unit test for cache synchronization behavior. |
| test-refactor/wh_test_list.c | Registers the new whTest_AuthSetPermsCacheSync test. |
| test-refactor/README.md | Updates test inventory documentation to include the new auth cache-sync test. |
| docs/src/5-Features.md | Documents core vs backend responsibility for permission policy and explains live-session binding semantics and consequences. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b0fbc8c to
99d30d3
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #481
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
No new issues found in the changed files. ✅
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 2 total — 1 posted, 1 skipped
Posted findings
- [Info] Session permission cache mirrors caller-supplied values, not backend-persisted state —
src/wh_auth.c:483-491
Skipped findings
- [Low] Session cache mirrors supplied permissions verbatim; diverges if a custom backend transforms them
Review generated by Skoll via Claude/Codex
e82d8e5 to
3af8cef
Compare
3af8cef to
5cfcc0a
Compare
Problem
wh_Auth_UserSetPermissionswrote the new permissions to the backend but neverrefreshed
context->user.permissions, the per-session cache thatwh_Auth_CheckRequestAuthorizationactually reads. A logged-in user thatlowered its own permissions kept the old, higher ones for the rest of the
session — revocation did not bind until the next login. An admin could
self-demote to conceal its privilege level while retaining full admin access.
Reachable from any client through
wh_Client_AuthUserSetPermissions. CWE-613.Closes f-4239.
Fix (
src/wh_auth.c)On backend success, when the target
user_idis this context's logged-in user,context->user.permissionsis refreshed, so the change binds on the very nextrequest — mirroring the self-synchronisation
wh_Auth_Logoutalready does.Seven lines; no other behavior change.
wolfhsm/wh_auth.hdocuments it on the API.docs/src/5-Features.mdadds a"Permission Changes and Live Sessions" section covering the immediate
self-demote (which can be self-locking) and the fact that other sessions keep
their cached permissions until they log in again.
Tests (
test-refactor/client-server/wh_test_auth.c)Driven through
wh_Client_*against the running POSIX server and the realwh_Auth_Base*backend, appended to the existingwhTest_AuthSetPermissions.No new file, no new entry point.
USER_ADDbit(keeping admin +
USER_SET_PERMISSIONSso it can restore) and its ownwh_Client_AuthUserAddis then denied. A change targeting a different userleaves the caller's session untouched.
USER_SET_PERMISSIONSissues three calls; all are refused withWH_ERROR_ACCESSby the admin-only backend, and neither stored record picksup the admin flag.
Each case captures its outcomes and restores the admin session before
asserting, so a failing assert cannot leave the shared server's admin baseline
demoted or its 5-slot user table full.
Verification
Merge base
a288dc4. Clean under-std=c90 -Werror -Wall -Wextra, zerowarnings.
test-refactor AUTH=1test AUTH=1(legacy)Negative control: removing the cache refresh fails
whTest_AuthSetPermissions(50/18/1); restored, it passes. Non-AUTH configs are unaffected — all of
wh_auth.csits behindWOLFHSM_CFG_ENABLE_AUTHENTICATION.